Security/EscapeOutput: add tests for namespaced names #2618
Conversation
|
Fixed and force-pushed. The problem was related to a change in how PHPCS tokenizes FQN exit/die. I added more details in a code comment. |
rodrigoprimo
force-pushed
the
escape-output-namespaced-names-tests
branch
from
bcab6ad to
490c471
Compare
|
Moved to draft pending the review of #2620 |
rodrigoprimo
force-pushed
the
escape-output-namespaced-names-tests
branch
from
490c471 to
0d6763d
Compare
…attern Add tests to ensure the `basename( __FILE__ )` pattern recognition in `_deprecated_file()` only applies to global `basename()` function calls, not to other constructs that might look similar.
rodrigoprimo
force-pushed
the
escape-output-namespaced-names-tests
branch
from
0d6763d to
322829b
Compare
|
@jrfnl, I'm moving this PR back to ready for review. |
jrfnl
left a comment
jrfnl
left a comment
There was a problem hiding this comment.
@rodrigoprimo Very good effort, needs a little more work, but these tests will help a lot!
| echo \get_search_query( true ); // Ok. | ||
| echo \get_search_query( false ); // Bad. | ||
| echo MyNamespace\get_search_query( true ); // Bad. | ||
| echo \MyNamespace\get_search_query( true ); // Bad. | ||
| echo namespace\get_search_query( true ); // Bad. The sniff should stop flagging this once it can resolve relative namespaces. | ||
| echo namespace\Sub\get_search_query( true ); // Bad. |
There was a problem hiding this comment.
Should there be tests here with FQN true/false and case-variations ? I.e. \TRUE and \False ?
| // PHPCS 3.13.3 changed the tokenization of FQN exit/die it impacts directly how this test case | ||
| // behaves (see https://github.com/PHPCSStandards/PHP_CodeSniffer/issues/1201). | ||
| 664 => version_compare( $phpcs_version, '3.13.3', '>=' ) ? 1 : 0, |
There was a problem hiding this comment.
I seem to remember we dropped support for PHPCS < 3.13.4 ? In which case, this toggle should no longer be needed.
| <?php | ||
|
|
||
| echo PHP_VERSION_ID, PHP_VERSION, PHP_EOL, PHP_EXTRA_VERSION; // OK. | ||
| echo PHP_VERSION_ID, PHP_VERSION, \PHP_EOL, PHP_EXTRA_VERSION; // OK. |
There was a problem hiding this comment.
Should there be tests with these "safe constants" in namespaced variants ?
There was a problem hiding this comment.
Would it make sense to also have some tests like the below ? (mind: I've not checked whether the function names in the second line (short echo) are the correct WP function names, this will need verification)
?>
<a href="<?php \the_permalink(); ?>" title="<?php \the_title_attribute(); ?>"><?php \the_title(); ?></a>
<a href="<?= \get_permalink(); ?>" title="<?= \get_title_attribute(); ?>"><?= \get_the_title(); ?></a>
<?phpThere was a problem hiding this comment.
And how about this ?
echo \true, \False, \NULL;I know, it's kind of silly code, but the tokens are in the "safe tokens" list and with the fully qualified bit, the behaviour may change ?
There was a problem hiding this comment.
I also see a separate category of array walking functions being handled. Do these need tests with the namespaced name variations too ?
There was a problem hiding this comment.
<a href="<?php \the_permalink(); ?>" title="<?php \the_title_attribute(); ?>"><?php \the_title(); ?></a>@jrfnl, correct me if I'm missing something, but I'm not sure what the value of adding the test above would be. The sniff bails early when it encounters any of those three functions, as they are not in the list of functions that this sniff looks for. the_title_attribute() is an auto-escaping function, and the_permalink() and the_title() used to be (removed in #1547). But the code above does not trigger the check for auto-escaping functions, and there are already tests for this group of functions. Maybe the original test on line 9 needs to be removed or updated? Or am I missing its purpose?
Regarding the second line you suggested, I made a minor modification, and I believe it makes sense to add it to test multiple short open tags and fully qualified functions.
<a href="<?= \get_permalink(); ?>" title="<?= \the_title_attribute( array( 'echo' => false ) ); ?>"><?= \get_the_title(); ?></a><!-- Bad x 2. -->
Co-authored-by: Juliette <[email protected]>
rodrigoprimo
force-pushed
the
escape-output-namespaced-names-tests
branch
from
7190fe2 to
d56798e
Compare
Co-authored-by: Juliette <[email protected]>
2 participants
Description
In preparation for PHPCS 4.0, which changes the tokenization of namespaced names, this PR adds tests with all variants of namespace names (unqualified, partially qualified, fully qualified, and namespace relative) to the
WordPress.Security.EscapeOutputsniff. This is a continuation of #2581, where tests were added for all sniffs extendingAbstractFunctionRestrictionsexceptEscapeOutput.This PR also includes tests to ensure the
basename( __FILE__ )pattern recognition in_deprecated_file()only applies to globalbasename()function calls, not to other constructs that might look similar (such as namespaced variants or class methods).